posix: Add headers related to BSD Sockets API #16621
Conversation
|
All checks are passing now. Review history of this comment for details about previous failed status. |
pfalcon
force-pushed
the
posix-socket-headers
branch
from
89a884f to
59631f4
Compare
pfalcon
force-pushed
the
posix-socket-headers
branch
from
59631f4 to
340f19d
Compare
There was a problem hiding this comment.
couple of things:
- empty headers should be removed
- at least one non-empty header doesn't seem to comply with the standard it's supposed to
Can you make a note of what SUS version you're using in these files and comply with the requirements of the standard? Otherwise I really don't understand the goal.
|
|
||
| #include <net/socket.h> | ||
|
|
||
| static inline char *inet_ntop(sa_family_t family, const void *src, char *dst, |
There was a problem hiding this comment.
Are you sure about this? These two functions don't seem to belong in this file: http://pubs.opengroup.org/onlinepubs/007908799/xns/arpainet.h.html
What SUS version are you targeting here?
There was a problem hiding this comment.
What SUS version are you targeting here?
Targeting latest publicly available POSIX version: http://pubs.opengroup.org/onlinepubs/9699919799/
http://pubs.opengroup.org/onlinepubs/9699919799/functions/inet_ntop.html
Quick check: "man inet_ntop"
There was a problem hiding this comment.
I haven't been following the posix compatibility work carefully, but this PR makes me wonder if it's just piecemeal efforts to support individual applications, or a disciplined approach that's enabling functionality in a staged way.
I'm not asking for the entire POSIX API to be implemented in one go. That's a ridiculous strawman.
I am asking what the structure of the approach is, given that this PR seems to have problems in that regard.
| * | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
| #ifndef ZEPHYR_INCLUDE_POSIX_NET_IF_H_ |
There was a problem hiding this comment.
IMO, having the header but not actually supporting the features it's specified to contain does more harm than good.
This comment applies multiple places in this PR.
There was a problem hiding this comment.
IMO, having the header but not actually supporting the features it's specified to contain does more harm than good.
What makes you think so? Do you want me to implement the whole POSIX (with extensions) in one PR? Sorry, that's not possible.
Otherwise, as the commit message says, "This set of headers is what is required to build
https://github.com/open62541/open62541 with Zephyr." So, without this (empty) header, the POSIX application specimen doesn't build, and with that header, even if empty, it builds. Judge the good.
There was a problem hiding this comment.
Otherwise, as the commit message says, "This set of headers is what is required to build
open62541/open62541 with Zephyr." So, without this (empty) header, the POSIX application specimen doesn't build, and with that header, even if empty, it builds. Judge the good.
so you are saying anyone building a 3rd party library/project should start adding empty headers in zephyr to satisfy the external build? This just does not make any sense.
There was a problem hiding this comment.
so you are saying anyone building a 3rd party library/project should start adding empty headers in zephyr to satisfy the external build?
Where did I say that? I said that locations and presence of POSIX headers is mandated by the POSIX standard, and 3rd-party software relies on that (and specific example is given). I apologize once again for not putting implementation of the entire POSIX in this PR, and instead just making dozenth small step in the direction of better support of it in Zephyr, like I did before and will intend to do further, for as long as my work supported. (I can easily put it on backburner and do instead things which my boss told me to do ;-) ).
There was a problem hiding this comment.
Otherwise, as the commit message says, "This set of headers is what is required to build
https://github.com/open62541/open62541 with Zephyr." So, without this (empty) header, the POSIX application specimen doesn't build, and with that header, even if empty, it builds. Judge the good.
Yes, I saw that.
If you're writing a patch to get this single application to compile, it's not ready for upstream. If Zephyr offers a POSIX header but don't implement or declare literally a single piece of its functionality just to get a single application to compile, it's just false advertising.
There was a problem hiding this comment.
If you're writing a patch to get this single application to compile, it's not ready for upstream.
No, that's not what I'm doing. See my reply to @nashif. I'm gradually improving POSIX subsystem, period.
If Zephyr offers a POSIX header but don't implement or declare literally a single piece of its functionality just to get a single application to compile, it's just false advertising.
The problem, Marti, is that Zephyr already advertises POSIX subsystem, which doesn't really work. Likewise, it advertises network stack which doesn't really work. That's the things I'm aware of. And I'm not sure about your current company, but I bet your previous one just relies on all that stuff to already work. Likewise, real top-level work I should be doing assumes that all this stuff just works. But it doesn't. I'm a bit shy to tell that "no, Zephyr can't do that at all/reliably" to 3rd-parties, that's why I submit these patches. However, I bet I need to go back to my real assignments for now after all.
There was a problem hiding this comment.
The problem, Marti, is that Zephyr already advertises POSIX subsystem, which doesn't really work.
can you please tell or educate me how any of the changes in this patch improve or touch the existing POSIX subsystem?
You are just adding headers that we never supported and never claimed to support. Zephyr never claimed it supports POSIX, our goal was limited to only a subset of PSE52, nothing more. We know there are some issues there, especially with headers and c libraries, but I do not see this fixing any of that, instead, and per your commit message, you are adding empty headers to make an external project build.
Period.
There was a problem hiding this comment.
can you please tell or educate me how any of the changes in this patch improve or touch the existing POSIX subsystem?
I've been doing that with a number of comments already. If it's still not clear, then perhaps the communication pipe is to narrow and other medium is required.
Zephyr never claimed it supports POSIX, our goal was limited to only a subset of PSE52, nothing more.
I don't know which party you refer to in "our goal". (Intel? Zephyr TSC?) But our goal, as Zephyr users and contributors, is to run existing, real-world software with Zephyr. There're tons of existing, useful, more or less quality software available in the industry, whereas developing new software from scratch takes years, and fixing issues with it takes decades. I'm not sure if "you" (the party you refer to) overlook this fact, or willfully want to force Zephyr user to use NIH, proprietary, misdesigned (just look how many of "native" Zephyr APIs get deprecated, or "new API for X" appear) APIs. In the latter case, we respectfully disagree. In the former case, we're happy to bring it to your attention. (See also #16683 and feel free to ask me for more user stories).
What's more interesting is that we (parties who want the above) were here all the time. For example, all 3 years I'm contributing to Zephyr, I'm working in the direction described above. (And of course, I do that not on my own whim, but as part of my work assignments.) So, it's truly insightful to finally (or for the Nth time?) let know about our existence and say hello.
you are adding empty headers to make an external project build.
I'm adding headers mandated by POSIX (big, real-world POSIX, not some fine-print TLA you put in "your" marketing materials). Yes, for this initial submission some of them empty. They are intended to be filled in gradually and incrementally, in the same way as development of any project goes. And here we switch again to process and community management mattes - either encouraging contributors by valuing their dedicated, even if incremental, work, or discouraging by write-offs like "empty files are BAAAD!!!11" (empty files are bad, most of the time; but sometimes mere presence of a file gives enough significance).
No, they shouldn't. What makes you think so? |
pfalcon
force-pushed
the
posix-socket-headers
branch
from
340f19d to
cd90de4
Compare
|
Rebased on master. |
|
In order to proceed with this PR, I think we could just remove the empty headers. They do not bring much benefit to this PR anyway. The BSD socket APIs would be needed in other PRs like #16683 so it would be important that this PR gets merged. |
They bring benefit to POSIX subsys (allow to build more software than without them, an example is given). So, if there's fixation on "empty headers are bad!!11", then I'd rather work on making those headers non-empty, rather than work on excluding them from this one-commit PR. As time permits. |
I am ok with those empty headers, but if there is lot of resistance, perhaps it is better to split the PR into two. Now this PR is blocking other stuff that needs to proceed. |
Are you sure it's blocking something? This set of PRs (#16621, #16626) was made as a response to Antmicro's folks saying "POSIX+Newlib is very broken, that's why we, instead of fixing it, work on duplicating Newlib functionality in minlibc". I wanted to show that, while it's broken, it's not that much effort to fix it up. I never heard them agree with me (that it makes sense to stop munging minlibc, and instead pull together with newlibc). Nor they commented here that it blocks their #16683 and voted it up to be merged. So yeah, it's a surprise that it blocks that PR. In either case, I'm on vacation by the end of day, so likely won't be able to do anything about this PR for the next week or so. |
If possible, could you join the networking forum today, we could discuss there how to proceed with this PR. |
We did back up from the minilibc effort, just as you suggested. And we definitely agree that improvements in Newlib are the way to go, I just mentioned that we cannot put resources in this effort at this moment. Our PR introduces all the required changes in the scope of a sample, not to meddle with any libc implementation. This PR is not a block for us per se Personally I think that merging empty headers such as these definitely makes sense. In the end, they are also a part of the API, it's not only function declarations - and it enables users of this particular part of the API. It would not make sense if the rest of the libc/posix API was cleaned up and it would just be a header with missing features, but it's not the case. |
Will be there. |
|
Yesterday in Networking forum, we discussed how to proceed with Posix API support and the consensus was that this needs to be done incrementally. It is just not feasible that we try to make everything ready in one go. About the empty header files issue, I think we should allow those in this case. As @PiotrZierhoffer mentioned, the files are also part of the API. If the app really needs functions in those empty header files, then of course there will be a compilation error. If on the other hand, the app just includes the files "just in case", the compilation will succeed in this case. If we do not allow empty header files, then the app would fail the compilation in both cases, which would be inferior outcome. @mbolivar how adamant are you with your -1 here? |
I think it's not only a "just in case" situation. Some defines, I believe, are scattered around different files in Zephyr, not exactly matching the standard. So when a "classic" POSIX-based app requires a header to obtain a definition, a Zephyr application may already have it from a different one. I don't have a good example here, but with all posix<->z_functions defines I think it's very possible. Of course this should be fixed, but I agree that doing it all at once is impossible. |
|
This issue was discussed in TSC today (3.7) and the opinion of TSC members was not to introduce empty headers. So we need to figure out what to do with this PR. Simplest option is to split this PR for time being. |
My main question to @pfalcon, which I'm not sure I've seen an answer to, is where this is all going. Is there a version of POSIX that we are targeting, and for which this commit is an intermediate step? Or is there just a random set of additions to the Zephyr headers that are being made to get individual applications to compile? In other words, are we truly proceeding incrementally, i.e. in small steps from point A to point B, or is POSIX support proceeding ad hoc, without a clear end point in mind? If this PR is blocking some other work, I am not adamant, but with my application developer hat on, I would question the value of ad hoc POSIX support in Zephyr, especially if we are going to claim support for the standard. |
No worries, as the TSC members were against introducing empty headers, we need to split this one in order to continue with other PRs. Either we wait until @pfalcon comes back from holiday or I can prepare a separate PR that just introduces necessary stuff. |
|
I took this PR and removed the empty headers, the result can be found at #17346 |
pfalcon
force-pushed
the
posix-socket-headers
branch
2 times, most recently
from
78c8211 to
e97b161
Compare
|
Great news - there're no empty headers in this patchset any longer. @mbolivar, Please re-review. |
|
Btw, regarding empty header files, see neighbor PR: https://github.com/zephyrproject-rtos/zephyr/pull/16626/files#r307337717 . So, we do have empty header files in the codebase. Which are eventually get filled in. Even by nobody else but me. So I again would like to thank everyone concerned about the empty header files (which were here previously), but there's little to worry about, it's the development process as usual. |
|
As this is adding new functionality would like more input from the users of this functionality and the need to get this merged for 2.0. (AntMicro, Systerel, etc) |
@tgorochowik, @PiotrZierhoffer, can you please review this. (Couldn't add @PiotrZierhoffer to reviewers, but you still can review by going to https://github.com/zephyrproject-rtos/zephyr/pull/16621/files and clicking green "Review changes" button on the right near the top of page). |
pfalcon
force-pushed
the
posix-socket-headers
branch
from
e97b161 to
e28a324
Compare
There was a problem hiding this comment.
I'm dropping my -1 on this.
I have read the RFC by @pfalcon stating that he intends the POSIX effort to continue in an ad-hoc fashion. I am generally against this idea -- as an application developer, "zephyr supports some random set of POSIX APIs" would not be extremely useful to me, especially without detailed documentation -- in .rst -- for what is and is not supported.
However, given the following, I'm changing my review status to "+0".
There was a problem hiding this comment.
As I don't think it's possible to have any other approach than the incremental one, and having "pending posix support" is better than "having no posix support",
Regarding the code itself - I am not a POSIX expert, but I cannot see any evident problems.
From the user perspective - there are different points of view for different users.
New users would probably benefit from an extensive documentation of features that are there or are missing, but it's difficult to maintain if the development process is rapid. The worst case is that the documentation will be outdated and will not present all the features out there.
Current users, trying to build specific applications, will be glad there are features they can use. If you're porting a POSIX-based software you don't take the code to carefully analyze each and every API call against the docs, you try to compile it and see where does it blow up. This PR reduces the number of such problems, so I definitely give my +1.
|
The approach is indeed quite controversial but it is a step in the right direction and does help in already existing use-cases. |
A few of these headers are currently empty and provided to avoid compiler errors when building existing software. This set of headers is what is required to build https://github.com/open62541/open62541 with Zephyr. Signed-off-by: Paul Sokolovsky <[email protected]>
pfalcon
force-pushed
the
posix-socket-headers
branch
from
e28a324 to
fb7b940
Compare
|
Rebased on the master, CI passes. @jukkar: Can we please merge this, as it is approved? |
A few of these headers are currently just re-export to existing net/socket.h (all of them are useful to avoid compiler errors when building existing software.)
This set of headers is what was required to build
https://github.com/open62541/open62541 with Zephyr, and will be useful for other projects too.
Signed-off-by: Paul Sokolovsky [email protected]